-
Notifications
You must be signed in to change notification settings - Fork 140
mcp/server.go: implement server-side logging throughout codebase #306
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@@ -50,6 +52,8 @@ type Server struct { | |||
type ServerOptions struct { | |||
// Optional instructions for connected clients. | |||
Instructions string | |||
// Logger is used for server-side logging. If nil, slog.Default() is used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nil should mean no logging.
@@ -108,9 +112,11 @@ func NewServer(impl *Implementation, opts *ServerOptions) *Server { | |||
if opts.UnsubscribeHandler != nil && opts.SubscribeHandler == nil { | |||
panic("UnsubscribeHandler requires SubscribeHandler") | |||
} | |||
l := ensureLogger(opts.Logger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline this into L119
@@ -181,6 +198,7 @@ func (t *SSEServerTransport) Connect(context.Context) (Connection, error) { | |||
} | |||
|
|||
func (h *SSEHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) { | |||
h.ensureLogger() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this only occurs in one place, inline it.
could this also extend to streamable http? |
@ramongalate I think that should be separate, since the transport is lower level. Maybe a field on StreamableHTTPOptions and StreamableServerTransport? I think you can make a separate PR, not dependent on this one. |
mcp/server.go: implement server‑side logging
*slog.Logger
through server paths to improve debuggabilityensureLogger
so loggers are never nilServer
,SSEHandler
/SSEServerTransport
; remove nil checksAll tests are passing and no functionality should change with this PR
Fixes #170